Add /api/v1/banned_users/counts #907
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new API endpoint /api/v1/banned_users/counts that returns statistics about users who received a "red" (banned) trust level over three time periods: the last day, week, and month. The endpoint queries the TrustLevelAuditLog table to count distinct users who were assigned a red trust level within each time period.
Changes:
- Added a new route
GET /api/v1/banned_users/countsmapping tostats#banned_users_counts - Implemented
banned_users_countsmethod in the StatsController that queries TrustLevelAuditLog for distinct users banned in the last day, week, and month
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| config/routes.rb | Adds new GET route for banned users counts endpoint in the API v1 namespace |
| app/controllers/api/v1/stats_controller.rb | Implements banned_users_counts action with three queries counting distinct banned users across different time periods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| render json: { trust_level: level, trust_value: User.trust_levels[level] } | ||
| end | ||
|
|
||
| def banned_users_counts |
There was a problem hiding this comment.
This endpoint lacks authentication/authorization. Similar stats endpoints in this controller either require authentication via the ensure_authenticated! before_action (like the show action on line 2) or are scoped to specific users. This endpoint exposes aggregate banned user statistics without any access control, which could be sensitive information. Consider adding authentication or moving this endpoint to the admin namespace (Api::Admin::V1) which has built-in admin authentication via AdminApiKey.
| day_count = TrustLevelAuditLog.where(new_trust_level: "red") | ||
| .where("created_at >= ?", day_ago) | ||
| .distinct | ||
| .count(:user_id) | ||
|
|
||
| week_count = TrustLevelAuditLog.where(new_trust_level: "red") | ||
| .where("created_at >= ?", week_ago) | ||
| .distinct | ||
| .count(:user_id) | ||
|
|
||
| month_count = TrustLevelAuditLog.where(new_trust_level: "red") | ||
| .where("created_at >= ?", month_ago) | ||
| .distinct | ||
| .count(:user_id) |
There was a problem hiding this comment.
The queries may not accurately count what the endpoint name suggests. Currently, these count distinct user_ids where a TrustLevelAuditLog entry with new_trust_level='red' was created in the time period. This means if a user was banned (red), unbanned (blue), and banned again (red) within the same period, they would be counted. Additionally, users who were already banned before the time period but had no new audit log entries during it won't be counted. Consider clarifying whether this should count: (1) users who were newly banned in the period, (2) total unique users who received any ban action in the period, or (3) users who are currently banned at the end of the period.
| day_count = TrustLevelAuditLog.where(new_trust_level: "red") | ||
| .where("created_at >= ?", day_ago) | ||
| .distinct | ||
| .count(:user_id) | ||
|
|
||
| week_count = TrustLevelAuditLog.where(new_trust_level: "red") | ||
| .where("created_at >= ?", week_ago) | ||
| .distinct | ||
| .count(:user_id) | ||
|
|
||
| month_count = TrustLevelAuditLog.where(new_trust_level: "red") | ||
| .where("created_at >= ?", month_ago) | ||
| .distinct | ||
| .count(:user_id) |
There was a problem hiding this comment.
This query pattern could have performance issues as the audit log table grows. The query filters by new_trust_level and created_at, but the database schema (db/schema.rb lines 555-568) doesn't have a composite index on (new_trust_level, created_at). The existing indexes are on (user_id, created_at) and (changed_by_id, created_at). Without an appropriate index, these queries will perform full table scans filtered by the WHERE clauses. Consider adding a composite index on (new_trust_level, created_at) to optimize this query pattern, or alternatively scope the query using existing indexes (e.g., by using user_id ranges with the indexed columns).
| day_count = TrustLevelAuditLog.where(new_trust_level: "red") | ||
| .where("created_at >= ?", day_ago) | ||
| .distinct | ||
| .count(:user_id) | ||
|
|
||
| week_count = TrustLevelAuditLog.where(new_trust_level: "red") | ||
| .where("created_at >= ?", week_ago) | ||
| .distinct | ||
| .count(:user_id) | ||
|
|
||
| month_count = TrustLevelAuditLog.where(new_trust_level: "red") | ||
| .where("created_at >= ?", month_ago) | ||
| .distinct | ||
| .count(:user_id) |
There was a problem hiding this comment.
The three queries are nearly identical except for the time comparison. This can be refactored to reduce code duplication and make it easier to maintain. Consider extracting the common query logic into a helper method that takes a time parameter.
| def banned_users_counts | ||
| now = Time.current | ||
|
|
||
| day_ago = now - 1.day | ||
| week_ago = now - 1.week | ||
| month_ago = now - 1.month | ||
|
|
||
| day_count = TrustLevelAuditLog.where(new_trust_level: "red") | ||
| .where("created_at >= ?", day_ago) | ||
| .distinct | ||
| .count(:user_id) | ||
|
|
||
| week_count = TrustLevelAuditLog.where(new_trust_level: "red") | ||
| .where("created_at >= ?", week_ago) | ||
| .distinct | ||
| .count(:user_id) | ||
|
|
||
| month_count = TrustLevelAuditLog.where(new_trust_level: "red") | ||
| .where("created_at >= ?", month_ago) | ||
| .distinct | ||
| .count(:user_id) | ||
|
|
||
| render json: { | ||
| day: day_count, | ||
| week: week_count, | ||
| month: month_count | ||
| } | ||
| end |
There was a problem hiding this comment.
This new endpoint lacks test coverage. All other endpoints in spec/requests/api/v1/stats_spec.rb have corresponding test cases that document their behavior and response schemas. Add test coverage for this endpoint that verifies the response structure and behavior, including test cases for different time periods.
1 participant
No description provided.